-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(gwe-est): add support for energy decay in the solid phase #2155
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user who contributed the test problem is mentioned by name. I assume it's ok if you have their permission?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you have time to chat about the test? I don't suspect a problem, but it would help me understand some of the details of the setup, and it would also be good to have a look at the results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nice cross-check might be to instead use ESL to add/remove heat to/from the model at the same total rate as in the current setup and make sure you get the same results for temperature. (Addition/removal of an amount of heat from the aquifer should be the same as production/decay of the same amount of heat distributed between the water and solid because the water and solid equilibrate anyway)
Another could be, starting with the existing setup, add/remove all the heat to/from just the water, and in another run all to/from just the solid, to make sure you get the same temperature results (assuming the same total rate of heat addition/removal in each case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth clarifying that the decay in each phase is a rate per amount of that phase (not per volume of aquifer), or is that obvious enough? A similar question applies to straight-up "decay" (sorption aside) in GWT, which is only in the water and is (presumably?) a rate per volume of water (not per volume of aquifer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The dimensions of decay for zero-order decay in the aqueous phase is energy per length cubed per time." -- Suggest modifying to "The dimensions of zero-order decay in the aqueous phase are energy per length cubed per time."
Similar suggestion for the analogous description of decay_solid.
Related question: based on the code and the test problem, the units of decay in the solid are energy per mass (of solid) per time, not "energy per length cubed per time"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth renaming subroutine est_fc_dcy to est_fc_dcy_water (similar for est_cq_dcy)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In est_cq_dcy, remove part of comment that says solid still needs to be added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove temporary comment notes about first-order decay from est_fc_dcy, est_cq_dcy, and est_cq_dcy_solid. If desired, could replace them with formal comments saying that first-order decay of energy is not supported. Also looks like there's a reference to first-order decay in read_options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove temporary comment note about negative temps from est_fc_dcy. If desired, could replace it with a formal comment about the fact that negative temps are currently not checked for or prevented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest modifying
if (.not. lname(2) .and. .not. lname(3)) then
write (errmsg, '(a)') 'Zero order decay in either the aqueous &
&or solid phase is active but zero-order rate coefficients, &
&either in the aqueous or solid phase, is not specified. &
&Either DECAY_WATER or DECAY_SOLID must be specified in the &
&griddata block.'
to this
if (.not. lname(2) .and. .not. lname(3)) then
write (errmsg, '(a)') 'Zero order decay in either the aqueous &
&or solid phase is active but the corresponding zero-order &
&rate coefficient is not specified. Either DECAY_WATER or &
&DECAY_SOLID must be specified in the griddata block.'
(shortened a bit, and corrected "coefficients ... is" to "coefficient ... is")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_zero_order_decay appears to limit decay to as to avoid negative temps, which is should not be doing. there is also a kluge note about wanting to use different expressions for the rates. we should chat about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe logical operators associate from left to right, so the following
elseif (this%idcy == 2 .and. this%idcysrc == 1 .or. &
this%idcysrc == 3) then ! zero order decay aqueous phase
which would evaluate the "and" first, then the "or", should be
elseif (this%idcy == 2 .and. (this%idcysrc == 1 .or. &
this%idcysrc == 3)) then ! zero order decay aqueous phase
Likewise,
elseif (this%idcy == 2 .and. this%idcysrc == 2 .or. &
this%idcysrc == 3) then ! zero order decay in the solid phase
should be
elseif (this%idcy == 2 .and. (this%idcysrc == 2 .or. &
this%idcysrc == 3)) then ! zero order decay in the solid phase
Support for specifying energy decay in the solid phase was missing despite it being described in the Supplemental Technical Information document. The best I can figure is that during development it was initially skipped over with the intention of coming back to it - but then was forgotten about.
ruff
on new and modified python scripts in .doc, autotests, doc, distribution, pymake, and utils subdirectories.fprettify